Fix the difference between aws_waf_byte_match_set and aws_wafregional_byte_match_set #5043

chroju · 2018-07-01T05:13:28Z

Changes proposed in this pull request:

Some aws waf and aws waf-regional API have same attributes each other. However, Terraform aws_waf_*** resources and aws_wafregional_*** resources have same attributes with the different name, one is singular and the other is plural. Although this is mentioned a little in #4137 , this discrepancy can also be seen in several other resources about AWS WAF(Regional).

This PR will fix such inconsistencies. I think it would be better to unify in singular names, because these attributes type is not list but map .

Output from acceptance testing:

WIP

$ make testacc TEST=./aws TESTARGS='-run=TestAccAWSWafByteMatchSet*'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAWSWafByteMatchSet* -timeout 120m
=== RUN   TestAccAWSWafByteMatchSet_basic
--- PASS: TestAccAWSWafByteMatchSet_basic (42.07s)
=== RUN   TestAccAWSWafByteMatchSet_changeNameForceNew
--- PASS: TestAccAWSWafByteMatchSet_changeNameForceNew (77.94s)
=== RUN   TestAccAWSWafByteMatchSet_changeTuple
--- PASS: TestAccAWSWafByteMatchSet_changeTuple (72.65s)
=== RUN   TestAccAWSWafByteMatchSet_noTuples
--- PASS: TestAccAWSWafByteMatchSet_noTuples (37.13s)
=== RUN   TestAccAWSWafByteMatchSet_disappears
--- PASS: TestAccAWSWafByteMatchSet_disappears (35.07s)
PASS
ok      github.com/chroju/terraform-provider-aws/aws    264.901s

bflad

Hi @chroju 👋 Thanks for submitting this. When renaming attributes, we need to be sure to keep backwards compatibility for at least one major version by deprecating the existing attribute. This change also requires documentation. Please see below for notes.

bflad · 2018-07-02T13:27:37Z

aws/resource_aws_waf_byte_match_set.go

@@ -24,7 +24,7 @@ func resourceAwsWafByteMatchSet() *schema.Resource {
 				Required: true,
 				ForceNew: true,
 			},
-			"byte_match_tuples": &schema.Schema{


We need to duplicate the entire existing byte_match_tuples attribute into the new byte_match_tuple attribute. Then in the existing byte_match_tuples attribute, we need to add some additional fields, e.g.

Computed: true, ConflictsWith: []string{"byte_match_tuple"}, Deprecation: "use `byte_match_tuple` instead",

bflad · 2018-07-02T13:29:24Z

aws/resource_aws_waf_byte_match_set.go

@@ -107,7 +107,7 @@ func resourceAwsWafByteMatchSetRead(d *schema.ResourceData, meta interface{}) er
 	}

 	d.Set("name", resp.ByteMatchSet.Name)
-	d.Set("byte_match_tuples", flattenWafByteMatchTuples(resp.ByteMatchSet.ByteMatchTuples))


When renaming attributes, we need to ensure both the old and new are still read into the Terraform state.

bflad · 2018-07-02T13:30:04Z

aws/resource_aws_waf_byte_match_set.go

@@ -117,8 +117,8 @@ func resourceAwsWafByteMatchSetUpdate(d *schema.ResourceData, meta interface{})

 	log.Printf("[INFO] Updating ByteMatchSet: %s", d.Get("name").(string))

-	if d.HasChange("byte_match_tuples") {


The existing logic for byte_match_tuples must be preserved in addition to the new byte_match_tuple

bflad · 2018-07-02T13:31:44Z

aws/resource_aws_waf_byte_match_set.go

@@ -24,7 +24,7 @@ func resourceAwsWafByteMatchSet() *schema.Resource {
 				Required: true,
 				ForceNew: true,
 			},
-			"byte_match_tuples": &schema.Schema{
+			"byte_match_tuple": &schema.Schema{


The documentation in website/docs/r/waf_byte_match_set.html.markdown should add the new attribute and add a deprecated notice to the existing attribute.

bflad · 2018-07-02T13:32:02Z

aws/resource_aws_waf_rule_test.go

@@ -402,15 +402,15 @@ func testAccAWSWafRuleConfig_changePredicates(name string) string {
 	return fmt.Sprintf(`
 resource "aws_waf_ipset" "ipset" {
  name = "%s"
-  ip_set_descriptors {


Was this change intentional?

Sorry, this is my mistake. Too mach changes were in the commit.

bflad · 2018-07-02T13:44:48Z

For what its worth, there can be multiple ByteMatchSet tuples as the attribute is schema.TypeSet and the updateByteMatchSetResource function works with multiple tuples. It seems this attribute should be plural.

chroju · 2018-07-03T14:01:01Z

@bflad Thanks for your review and comments.

I have noticed that this is so complex and breaking change than I thought. There are much more inconsistent attributes about AWS WAF and AWS WAFRegional, and it is debatable whether each of them should be plural or singular.

So, at first I would fix this PR not about these all inconsistent attributes but only byte_match_tuples . Also I will make the list of inconsistent attributes at #4137 once. Is this OK ?

bflad · 2018-07-03T14:26:49Z

So, at first I would fix this PR not about these all inconsistent attributes but only byte_match_tuples . Also I will make the list of inconsistent attributes at #4137 once. Is this OK ?

The maintainers would love this. ❤️ Design decisions in the issue and single resource/data source pull requests (easier to review/test quickly) is perfect.

Thanks for tackling this, its quite complex!

chroju · 2018-07-08T12:17:59Z

@bflad

I have reflected your review.

All changes in my commit is about aws_waf(regional)_byte_match_set only, and I have fixed the title of this PR too.
You mentioned byte_match_tuple(s) should be plural form (and I think so) , so I have fixed the singular form attribute in aws_wafregional_byte_match_set but not aws_waf_byte_match_set .

bflad

LGTM -- this should cover properly deprecating the old attribute and migrating it to the new one. Thanks for this @chroju! I'll include an additional note in the CHANGELOG about this deprecation as well.

5 tests passed (all tests)
=== RUN   TestAccAWSWafRegionalByteMatchSet_disappears
--- PASS: TestAccAWSWafRegionalByteMatchSet_disappears (8.93s)
=== RUN   TestAccAWSWafRegionalByteMatchSet_noByteMatchTuples
--- PASS: TestAccAWSWafRegionalByteMatchSet_noByteMatchTuples (11.31s)
=== RUN   TestAccAWSWafRegionalByteMatchSet_basic
--- PASS: TestAccAWSWafRegionalByteMatchSet_basic (16.36s)
=== RUN   TestAccAWSWafRegionalByteMatchSet_changeNameForceNew
--- PASS: TestAccAWSWafRegionalByteMatchSet_changeNameForceNew (24.54s)
=== RUN   TestAccAWSWafRegionalByteMatchSet_changeByteMatchTuples
--- PASS: TestAccAWSWafRegionalByteMatchSet_changeByteMatchTuples (39.42s)

bflad · 2018-07-11T19:36:17Z

This has been released in version 1.27.0 of the AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

nbrys · 2018-09-07T12:41:37Z

After changing

byte_match_tuple { text_transformation = "NONE" target_string = "/favicon.png" positional_constraint = "EXACTLY" field_to_match { type = "URI" } }

into:
byte_match_tuples { text_transformation = "NONE" target_string = "/favicon.png" positional_constraint = "EXACTLY" field_to_match { type = "URI" } }

I get the following change in terraform:
byte_match_tuple.511953057.field_to_match.#: "1" => "0"
byte_match_tuple.511953057.field_to_match.3756326843.data: "" => ""
byte_match_tuple.511953057.field_to_match.3756326843.type: "URI" => ""
byte_match_tuple.511953057.positional_constraint: "EXACTLY" => ""
byte_match_tuple.511953057.target_string: "/favicon.ico" => ""
byte_match_tuple.511953057.text_transformation: "NONE" => ""
byte_match_tuples.#: "0" => "1"
byte_match_tuples.511953057.field_to_match.#: "0" => "1"
byte_match_tuples.511953057.field_to_match.3756326843.data: "" => ""
byte_match_tuples.511953057.field_to_match.3756326843.type: "" => "URI"
byte_match_tuples.511953057.positional_constraint: "" => "EXACTLY"
byte_match_tuples.511953057.target_string: "" => "/favicon.ico"
byte_match_tuples.511953057.text_transformation: "" => "NONE"

But I end up with an empty filter in WAF

ghost · 2020-04-03T17:42:38Z

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks!

ghost added the size/L Managed by automation to categorize the size of a PR. label Jul 1, 2018

bflad added breaking-change Introduces a breaking change in current functionality; usually deferred to the next major release. service/waf Issues and PRs that pertain to the waf service. technical-debt Addresses areas of the codebase that need refactoring or redesign. labels Jul 2, 2018

bflad suggested changes Jul 2, 2018

View reviewed changes

bflad added the waiting-response Maintainers are waiting on response from community or contributor. label Jul 5, 2018

resource/aws_wafregional_byte_match_set: Rename byte_match_tuple

7b945c5

ghost added the size/L Managed by automation to categorize the size of a PR. label Jul 7, 2018

chroju changed the title ~~[WIP] Fix some differences about AWS WAF and AWS WAFRegioal resources~~ [WIP] Fix the difference between aws_waf_byte_match_set and aws_wafregional_byte_match_set Jul 8, 2018

chroju changed the title ~~[WIP] Fix the difference between aws_waf_byte_match_set and aws_wafregional_byte_match_set~~ Fix the difference between aws_waf_byte_match_set and aws_wafregional_byte_match_set Jul 8, 2018

chroju mentioned this pull request Jul 8, 2018

Differences between aws_waf_web_acl and aws_wafregional_web_acl #4137

Closed

bflad removed breaking-change Introduces a breaking change in current functionality; usually deferred to the next major release. waiting-response Maintainers are waiting on response from community or contributor. labels Jul 9, 2018

bflad added this to the v1.27.0 milestone Jul 9, 2018

bflad approved these changes Jul 9, 2018

View reviewed changes

bflad merged commit 66b5d55 into hashicorp:master Jul 9, 2018

bflad added a commit that referenced this pull request Jul 9, 2018

Update CHANGELOG for #5043

7678449

ghost locked and limited conversation to collaborators Apr 3, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix the difference between aws_waf_byte_match_set and aws_wafregional_byte_match_set #5043

Fix the difference between aws_waf_byte_match_set and aws_wafregional_byte_match_set #5043

chroju commented Jul 1, 2018

bflad left a comment

bflad Jul 2, 2018 •

edited

Loading

bflad Jul 2, 2018

bflad Jul 2, 2018

bflad Jul 2, 2018

bflad Jul 2, 2018

chroju Jul 3, 2018

bflad commented Jul 2, 2018

chroju commented Jul 3, 2018

bflad commented Jul 3, 2018

chroju commented Jul 8, 2018

bflad left a comment

bflad commented Jul 11, 2018

nbrys commented Sep 7, 2018 •

edited

Loading

ghost commented Apr 3, 2020

		@@ -117,8 +117,8 @@ func resourceAwsWafByteMatchSetUpdate(d *schema.ResourceData, meta interface{})

		log.Printf("[INFO] Updating ByteMatchSet: %s", d.Get("name").(string))

		if d.HasChange("byte_match_tuples") {

Fix the difference between aws_waf_byte_match_set and aws_wafregional_byte_match_set #5043

Fix the difference between aws_waf_byte_match_set and aws_wafregional_byte_match_set #5043

Conversation

chroju commented Jul 1, 2018

bflad left a comment

Choose a reason for hiding this comment

bflad Jul 2, 2018 • edited Loading

Choose a reason for hiding this comment

bflad Jul 2, 2018

Choose a reason for hiding this comment

bflad Jul 2, 2018

Choose a reason for hiding this comment

bflad Jul 2, 2018

Choose a reason for hiding this comment

bflad Jul 2, 2018

Choose a reason for hiding this comment

chroju Jul 3, 2018

Choose a reason for hiding this comment

bflad commented Jul 2, 2018

chroju commented Jul 3, 2018

bflad commented Jul 3, 2018

chroju commented Jul 8, 2018

bflad left a comment

Choose a reason for hiding this comment

bflad commented Jul 11, 2018

nbrys commented Sep 7, 2018 • edited Loading

ghost commented Apr 3, 2020

bflad Jul 2, 2018 •

edited

Loading

nbrys commented Sep 7, 2018 •

edited

Loading